Update <link> static assets to use the new domain #8916
Conversation
jimjohnsonrollings
added
ws-articles
| sizes: '96x96', | ||
| }, | ||
| { | ||
| href: | ||
| 'https://news.files.bbci.co.uk/include/articles/public/news/images/icons/icon-128x128.png', | ||
| 'https://static.files.bbci.co.uk/ws/simorgh-assets/public/news/images/icons/icon-128x128.png', |
There was a problem hiding this comment.
The cache-control header max-age is different
Old endpoint:
curl -Is https://static.files.bbci.co.uk/ws/simorgh-assets/public/news/images/icons/icon-128x128.png
cache-control: max-age=31622400 (366 days)
New endpoint:
curl -Is https://news.files.bbci.co.uk/include/articles/public/news/images/icons/icon-128x128.png
cache-control: max-age=604800 (7 days)
and a new header:
strict-transport-security: max-age=2592000
There was a problem hiding this comment.
The cache control change is I think deliberate - we didn't want so long on the new one so that we can more easily update the images, and settled on seven days as reasonably short without being too short.
I don't know about the other one, but maybe @chris-hinds or @jamesdonoh do?
There was a problem hiding this comment.
Okay. To enable updates to logo files, I'd suggest a better model would be to include a hash of the file content in the filename and keep a long cache-control max-age.
There was a problem hiding this comment.
Ok thanks.
Given this PR is just updating this URL to be consistent with the ones already in the manifest files, would it be okay to proceed with this as is, and I can write a new issue to cover considering the caching approaches?
There was a problem hiding this comment.
So long as the team is happy with the cache hit going from 366 days to 7, sure.
Regarding the new header strict-transport-security: max-age=2592000 (30 days) - this tells the browser to use HTTPS instead of HTTP for 30 days. This could also be extended to 366 days, since I don't believe this is expected to change.
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Strict-Transport-Security
There was a problem hiding this comment.
The cache control change is I think deliberate - we didn't want so long on the new one so that we can more easily update the images, and settled on seven days as reasonably short without being too short.
I don't know about the other one, but maybe @chris-hinds or @jamesdonoh do?
Apologies have only just seen this. Yes the cache changes were deliberate to ensure that we have a reasonable cache time but are able to still update the images.
Setting a hash in the url is a good idea but something we'd have to work on when the team has more time.
Regarding setting strict-transport-security we should work with the Mozart team on this and perhaps this is a default header that should be applied if it isn't already.
There was a problem hiding this comment.
As per what Chris has said, happy for this PR to go out as is - we can pick up other work relating to the cache afterwards.
There was a problem hiding this comment.
I've drafted an issue here for the caching stuff: #8931
chris-hinds
dismissed
sareh’s stale review
This work will be done as part of another PR
Resolves #8915
Overall change:
Updates the URL used by tags in all pages to point to new URLs for those assets.
Code changes:
cross-teamlabel to this PR if it requires visibility across World Service teamsTesting:
CYPRESS_APP_ENV=local CYPRESS_SMOKE=false yarn test:e2e:interactive)